Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Public Spec for Microsoft.Windows.Storage.Pickers [Milestone 1] #5155

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DinahK-2SO
Copy link
Contributor

The public Spec for Microsoft.Windows.Storage.Pickers

Preview: Microsoft.Windows.Storage.Pickers.md

| **Attribute** | **Type** | **Description** |
|--------------------------|---------------------------------------------------------|--------------------------------------------------------------------------|
| `ViewMode` | [Microsoft::Windows::Storage::Pickers::PickerViewMode](./PickerViewMode.md) | Gets or sets the view mode that the file picker is using to present items. |
| `SuggestedStartLocation` | [Microsoft::Windows::Storage::Pickers::PickerLocationId](./PickerLocationId.md)| Gets or sets the initial location where the file picker looks for files. |

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance SuggestedStartLocationDirectory will be added, where you can provide any string file path (e.g. if SuggestedStartLocation == PickerLocationId.Unspecified)? (see #4942 (comment))

It would be a bummer if this limitation, that only applies to the UWP picker and UWP apps and not the COM picker, would be retained. From history, if it's not being addressed now, it will forever stay like that :/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw: It's certainly a feature that would be much appreciated as @whiskhub said. There is a quite popular issue dotnet/maui#9212 on the MAUI side asking for this feature. A person did a great research into it here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, a SuggestedStartLocationDirectory would be very useful. If you don't implement that, please at least implement the SettingsIdentifier, otherwise the start location will be even less customizable than in UWP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi WinAppSDK friends @whiskhub , @MartyIX , @benstevens48 .

The string input of SuggestedStartLocation feature is on our wishlist for future milestones. For the first version of Microsoft.Windows.Storage.Pickers, we aim to implement method signatures consistent with Windows.Storage.Pickers and will not add new signatures at this time.

Copy link
Member

@DrusTheAxe DrusTheAxe Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and will not add new signatures at this time

You are, to a degree, as the spec is a consistent subset of Windows.Storage.Pickers - no PickMultipleFilesAsync(), CreateForUser(), etc. Why the omissions if consistency is a goal?*

* I can see why the Continue properties/methods are omitted. It's the others I find puzzling if consistency is the goal.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just afraid that "wishlist for future milestones" practically means "never" 😅 which means I'd "never" be able to use this picker :/

@benstevens48
Copy link

Is the general principle in WindowsAppSDK to continue using the StorageFile and StorageFolder types? I mean if writing in C#, using the .Net IO functions is more convenient, in which case it's preferable just to have file paths and not have the overhead of the StorageFile/Folder instances. However, I have to admit, I'm not full up to date with all the possible security models that can be used with WindowsAppSDK, so maybe there's a reason for using StorageFile and StorageFolder instead of just returning the file path?

@mominshaikhdevs
Copy link

mominshaikhdevs commented Feb 16, 2025

#8 (comment)

a new picker API that must have these two bare minimum features:

  1. works both in AppContainer/win32-app-isolation and outside of AppContainer.

  2. as fast as raw win32 apis.

EDIT:

longstanding issues that will be finally resolved : #88, #2504, #3536, microsoft/microsoft-ui-xaml#9557

| PicturesLibrary | 6 | The Pictures library. |
| VideosLibrary | 7 | The Videos library. |
| Objects3D | 8 | The Objects library. |
| Unspecified | 9 | An unspecified location. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the behavior?

"An unspecified location" means the caller doesn't specify something. So what's the behavior when this occurs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DrusTheAxe , I will add more details in the spec.
An unspecified location means that the file picker does not preset any specific storage location, allowing the user to choose the storage location themselves.

PS: The values here are consistent with Windows.Storage.Pickers.PickerLocationId


| **Name** | **Value** | **Description** |
|--------------------|-----------|----------------------------------------|
| DocumentsLibrary | 0 | The Documents library. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DocumentsLibrary = 0 means this is the default behavior?

Copy link
Contributor Author

@DinahK-2SO DinahK-2SO Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DocumentsLibrary = 0 means this is the default behavior?

Yes and no. For the enum itself, indeed it is 0 when not specified.
However, in the code of our new pickers, all 3 pickers set the default location as Unspecified.
We will implement:

struct FileOpenPicker : FileOpenPickerT<FileOpenPicker>
{
    private:
        // ... 
        PickerLocationId m_suggestedStartLocation{ PickerLocationId::Unspecified };

You can find detailed code on dev branch: https://github.com/microsoft/WindowsAppSDK/blob/user/xianghong/storage-pickers-develop/dev/Interop/StoragePickers/FileOpenPicker.h

Copy link
Member

@DrusTheAxe DrusTheAxe Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Unspecified is the default why is the value for Unspecified != 0

Defaults don't have to be 0/false/null but it's convenient. As this is new APIs with new types seems the default could have a value of 0. Is there a specific reason it doesn't or can't?

P.S. Regardless the answer the spec needs an update specifying the default behavior.


# Fields

| **Name** | **Value** | **Description** |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is Specified option? How do I provide an arbitrary path of my choosing?

Start location should be specifiable as a string

string path = GetPathIWantUseAsAString();
var openPicker = new FileOpenPicker(this.AppWindow.Id);
openPicker.SuggestedStartPath = path;
var file = await openPicker.PickSingleFileAsync();

Optionally it can also be specified as a StorageFolder

StorageFolder folder = GetPathIWantUseAsAStorageFolder();
var openPicker = new FileOpenPicker(this.AppWindow.Id);
openPicker.SuggestedStartFolder = folder;
var file = await openPicker.PickSingleFileAsync();

And if 2+ are specified what happens?

SUGGESTION: Given location (enum) + path (string) + folder (StorageFolder) if 2+ are specified they must be consistent when PickSingleFolderAsync/etc are called or E_INVALIDARG


# Fields

| **Name** | **Value** | **Description** |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a predefined enum rather than a string (or string + StorageFolder)?

Windows.Storage.KnownFolders has 13 properties for predefined / 'well known' locations.

The list here was 10 (+'Unspecified') but only 6 in common with KnownFolders (7 missing).

KNOWNFOLDERID used by SHGetKnownFolderPath supports 129 locations.

CONSIDER: Replacing the enum with string SuggestedStartPath, and maybe also supporting StorageFolder SuggestedStartFolder. See other comment for more details.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A current "UWP (AppContainer) app" may not have the Videos Library capability, but can still open a file picker in the Videos Library using this enum. Setting a SuggestedStartFolder directly using KnownFolder.VideosLibrary would throw access denied however.

So, probably some kind of enum must stay for AppContainer apps.

Copy link

@mominshaikhdevs mominshaikhdevs Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. UWPs/(aka CoreWindows).
  2. AppContainer.
  3. MSIX.

are totally three independent things altogether.

(#4844 (comment) + microsoft/winget-cli#4260 (comment))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but can still open a file picker in the Videos Library using this enum

Oh? That's interesting. Why?

Because the picker is interactive so OK on the prompt implicitly has user consent? If the app wants to access a location which normally requires an AppContainer to declare a capability but the user interactively navigates there that's OK as it implies user consent?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh? That's interesting. Why?

I can only guess, since I am not a Microsoft employee. 😅

auto& folder = co_await folderPicker.PickSingleFolderAsync();
if (folder != nullptr)
{
auto path = folder.Path();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto path{ folder.Path() };

ES.23: Prefer the {}-initializer syntax

}
```

C++
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You provided a C# example. Why also a C++ example?

The norm is a C# example and only add other languages if they pose additional or different needs beyond mere syntax. Here' the C# and C++ examples are the same just language syntax differences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @DrusTheAxe

The reason for including both C# and C++ examples in the documentation is to cater to a broader audience. While the examples may only differ in syntax, providing both versions can help developers who are more comfortable with one language over the other.

This approach is not unique to our specs. We have several other documents that provide examples in both languages, such as the CameraCaptureUI.md


C#

```csharp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think c# is preferable to csharp as it's supported by more environments (GitHub, ADO, VSCode, ...)

@codendone do you recall that conversation we had. Did it end with c# is the best choice or am I misremembering?

same comment applies throughout

Displays a UI element that allows the user to choose a folder.

### Definition
```cpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WinRT APIs should be defined as MIDL3.

Examples are written in specific languages (usually C#). Non-WinRT APIs are defined in their specific language, in the rare event a WinAppSDK API isn't WinRT.

RECOMMEND: Define as MIDL3, not any specific language

namespace Windows.Storage.Pickers
{
    Windows.Foundation.IAsyncOperation<Windows.Storage.StorageFolder> PickSingleFolderAsync();
}

@@ -0,0 +1,172 @@
FileOpenPicker Class
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec is split across multiple files, with their definitions (IDL) likewise split across multiple files. This complicates reviewing (and holistically understanding) the design and details.

Do you intend to have multiple .idl files in the implementation?

CONSIDER: Moving all the definitions to a single file, ideally as an # API section in Microsoft.Windows.Storage.Pickers.md

...
# API

```c# (but really MIDL3)
namespace Windows.Storage.Pickers
{
    ...
}

That will also address gaps e.g. where's the API contract details?

@DinahK-2SO DinahK-2SO changed the title Public Spec for Microsoft.Windows.Storage.Pickers Public Spec for Microsoft.Windows.Storage.Pickers [Milestone 1] Feb 17, 2025
@DinahK-2SO
Copy link
Contributor Author

Is the general principle in WindowsAppSDK to continue using the StorageFile and StorageFolder types? I mean if writing in C#, using the .Net IO functions is more convenient, in which case it's preferable just to have file paths and not have the overhead of the StorageFile/Folder instances. However, I have to admit, I'm not full up to date with all the possible security models that can be used with WindowsAppSDK, so maybe there's a reason for using StorageFile and StorageFolder instead of just returning the file path?

Hello @benstevens48 , The new API returns StorageFile and StorageFolder primarily to maintain consistency with the APIs in Windows.Storage.Pickers that it is intended to replace. This approach allows users currently utilizing Windows.Storage.Pickers to migrate to Microsoft.Windows.Storage.Pickers with minimal changes.

At the current stage, once we have the StorageFile, we can use StorageFile.Path for subsequent operations (as demonstrated in the examples). In future, we may consider adding more methods and return types.

@benstevens48
Copy link

Is the general principle in WindowsAppSDK to continue using the StorageFile and StorageFolder types? I mean if writing in C#, using the .Net IO functions is more convenient, in which case it's preferable just to have file paths and not have the overhead of the StorageFile/Folder instances. However, I have to admit, I'm not full up to date with all the possible security models that can be used with WindowsAppSDK, so maybe there's a reason for using StorageFile and StorageFolder instead of just returning the file path?

Hello @benstevens48 , The new API returns StorageFile and StorageFolder primarily to maintain consistency with the APIs in Windows.Storage.Pickers that it is intended to replace. This approach allows users currently utilizing Windows.Storage.Pickers to migrate to Microsoft.Windows.Storage.Pickers with minimal changes.

At the current stage, once we have the StorageFile, we can use StorageFile.Path for subsequent operations (as demonstrated in the examples). In future, we may consider adding more methods and return types.

@DinahK-2SO Thanks for the reply. That makes sense. I'm just concerned about the ongoing usage of StorageFile in WIndowsAppSDK since it always had very poor performance in UWP (I mean just creating 1 StorageFile could consume 100kB of RAM and loading many StorageFiles was hundreds of times slower than using Win32/.Net APIs). Although I haven't tested in in WIndowsAppSDK , so maybe it behaves differently. Probably in a file picker it doesn't matter too much, although in some cases the user may pick 1000s of files (like all photos in a large folder of photos etc), so it could start to become an issue.

@DrusTheAxe
Copy link
Member

Is the general principle in WindowsAppSDK to continue using the StorageFile and StorageFolder types? I mean if writing in C#, using the .Net IO functions is more convenient, in which case it's preferable just to have file paths and not have the overhead of the StorageFile/Folder instances. However, I have to admit, I'm not full up to date with all the possible security models that can be used with WindowsAppSDK, so maybe there's a reason for using StorageFile and StorageFolder instead of just returning the file path?

Hello @benstevens48 , The new API returns StorageFile and StorageFolder primarily to maintain consistency with the APIs in Windows.Storage.Pickers that it is intended to replace. This approach allows users currently utilizing Windows.Storage.Pickers to migrate to Microsoft.Windows.Storage.Pickers with minimal changes.

Can you provide a variant which returns the file as a string e.g. string PickSingleFileNameAsync(...)?

Properly implementing that would require more than skin deep differences but ahort term that could be implemented as

var storageFile = picker.PickSingleFileAsync();
return storageFile != null ? storageFile.Path : null;

with the deeper implementation optimized to return the string w/o any StorageFile in the callstack. Even if only syntactic candy it lays the foundation for the improved dev experience while deferring the deeper optimized work for later when time permits.

@DrusTheAxe
Copy link
Member

@DinahK-2SO Performance can be a concern if you have the path/file as a string. The cost to create a StorageFile/Folder object can be non-trivial if your sole purpose is to rip the string out of the returned object and free the object.

Is the sole purpose of this spec is to provide WinAppSDK alternatives supporting elevation to Windows.* APIs that don't?

Seems it's maybe that and less given what's spec'd is a subset of Windows.* pickers (no PickMultipleFilesAsync, CreateForUser, etc). It's hard assess the spec without an understanding of the desired endgame. Is that longer-term clear to you but intentionally deferred for now, 'cause, reasons?

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • specs/StoragePickers/FileSavePicker.md: Evaluated as low risk
Comments suppressed due to low confidence (1)

specs/StoragePickers/FileOpenPicker.md:65

  • The method name PickSingleFilesAsync is incorrect. It should be renamed to PickSingleFileAsync.
## FileOpenPicker.PickSingleFilesAsync

Comment on lines +64 to +65
Return null if the file dialog was cancelled or closed without selection.

Copy link
Preview

Copilot AI Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should be 'Returns null if the file dialog was cancelled or closed without selection.'

Suggested change
Return null if the file dialog was cancelled or closed without selection.
Returns null if the file dialog was cancelled or closed without selection.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants